Skip to content

fix(graphile-postgis): wrap GeoJSON spatial filter inputs with ST_GeomFromGeoJSON (#724)#990

Merged
pyramation merged 2 commits intomainfrom
feat/fix-postgis-geojson-binding
Apr 17, 2026
Merged

fix(graphile-postgis): wrap GeoJSON spatial filter inputs with ST_GeomFromGeoJSON (#724)#990
pyramation merged 2 commits intomainfrom
feat/fix-postgis-geojson-binding

Conversation

@pyramation
Copy link
Copy Markdown
Contributor

@pyramation pyramation commented Apr 17, 2026

Summary

Fixes #724. All 26 PostGIS spatial operators registered by createPostgisOperatorFactory() were relying on the default connection-filter resolveSqlValue pipeline, which binds the GeoJSON object as raw text cast to ::geometry / ::geography. PostgreSQL's geometry_in / geography_in parsers do not accept GeoJSON text — they require ST_GeomFromGeoJSON(...) wrapping. Every spatial filter with a GeoJSON input therefore failed at runtime with parse error - invalid geometry.

The change, per operator registration:

  • Override resolveSqlValue: () => sql.null to disable the default bind.
  • In resolve(...), wrap the input via <schema>.st_geomfromgeojson($1::text), appending ::<schema>.geography when the registration is on a geography-based type.

This is the same pattern already used correctly by within-distance-operator.ts — now applied consistently across the broader operator set.

Unit tests in connection-filter-operators.test.ts are rewritten to assert the new contract: every operator's compiled SQL must contain the schema-qualified st_geomfromgeojson($1::text) wrap, and geography-type registrations must additionally append the ::geography cast.

The full end-to-end regression suite (live PG, 66 tests across all spatial operators × 7 GeoJSON input shapes × both codecs) lands via PR #989 and flips green once this is merged.

Updates since last revision

  • Added graphile/graphile-postgis to the CI matrix in .github/workflows/run-tests.yaml so the ST_GeomFromGeoJSON wrapping contract (the 218-test unit suite for this package, now including the new assertions) runs on every PR — previously the package was not in the matrix, which is how fix: bypass grafast middleware in graphile-test to preserve adapted withPgClient #724 slipped through in the first place. All 47 matrix jobs pass on this PR head.

Review & Testing Checklist for Human

  • Confirm the geography cast logic is correctspec.baseType === 'geography' appends ::<schema>.geography after the ST_GeomFromGeoJSON(...) wrap. Verify this produces the geography overload of each ST_* function rather than the geometry one, especially for operators registered on both codecs (intersects, covers, coveredBy, exactlyEquals, bboxIntersects2D).
  • Confirm sql.identifier(schemaName, 'st_geomfromgeojson') and sql.identifier(schemaName, 'geography') resolve correctly when PostGIS is installed to a non-public schema — there's a unit test for a postgis schema, but no live integration test in this PR. (PR test(postgis): regression coverage for GeoJSON spatial filter binding (#724) #989 exercises live PG but assumes the default public schema layout.)
  • Spot-check a couple of previously-working callers (e.g. anything that was passing a raw WKT string or geometry-typed bind via its own pipeline) to confirm they haven't regressed. The default pipeline is now disabled for all 26 operators.

Notes

  • The within-distance operator is not touched here — it already had the correct wrapping.
  • During the test audit we discovered a separate pre-existing issue: WithinDistanceInput is registered by graphile-postgis but does not surface on the generated GeometryInterfaceFilter schema type. This is unrelated to the GeoJSON binding bug and is tracked as a follow-up; the two test cases in the regression suite that exercise withinDistance are marked xit with a FIXME trail.
  • This PR is scoped to the fix only. The regression test suite lives in PR test(postgis): regression coverage for GeoJSON spatial filter binding (#724) #989, which is designed to land red on main until this fix is merged and then rebased → green.

Link to Devin session: https://app.devin.ai/sessions/c5eeee65a3c546c4ac6753bb05fa03e0
Requested by: @pyramation

… spatial operators

Fixes #724.

All 26 PostGIS spatial operators registered by
createPostgisOperatorFactory() were relying on the default
resolveSqlValue pipeline, which binds the GeoJSON object as raw text
cast to ::geometry / ::geography. Postgres' geometry_in and
geography_in parsers do not accept GeoJSON text — they require
ST_GeomFromGeoJSON() wrapping. Any spatial filter with a GeoJSON input
therefore failed at runtime with 'parse error - invalid geometry'.

Every spatial operator now overrides resolveSqlValue to sql.null and
wraps the input with ST_GeomFromGeoJSON($1::text) in the resolve
function, with an optional ::geography cast for geography-based
operators. This matches the pattern already used correctly by
within-distance-operator.ts.

Unit tests in connection-filter-operators.test.ts are updated to assert
the new ST_GeomFromGeoJSON wrapping contract. The broader regression
suite (graphql/orm-test/__tests__/postgis-spatial.test.ts) lands via
PR #989 and flips green once this change is merged.
@devin-ai-integration
Copy link
Copy Markdown
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@socket-security
Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addedpgpm@​1.4.27510010098100

View full report

@pyramation pyramation merged commit 1d89fd0 into main Apr 17, 2026
50 checks passed
@pyramation pyramation deleted the feat/fix-postgis-geojson-binding branch April 17, 2026 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant